Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some enhacements to is_loaded_directly #1880

Merged
merged 6 commits into from
Oct 29, 2024
Merged

Some enhacements to is_loaded_directly #1880

merged 6 commits into from
Oct 29, 2024

Conversation

lgoettgens
Copy link
Collaborator

@lgoettgens lgoettgens commented Oct 28, 2024 •
edited
Loading

  • add debug output
  • fix on julia nightly
  • add some helper code that makes fixing for future nightly changes easier

cc @benlorenz

@codecov Codecov
Copy link

codecov bot commented Oct 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (814f22b) to head (91af680).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.jl 95.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1880   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files         120      120           
  Lines       30260    30278   +18     
=======================================
+ Hits        26669    26687   +18     
  Misses       3591     3591

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benlorenz
Copy link
Collaborator

Why not use the existing banner tests, and maybe adjust / extend them a bit? The code there seems very similar to the new helper script.
E.g. add extra tests checking the output of is_loaded_directly and maybe just filter these lines for checking the banners later.

@lgoettgens
Copy link
Collaborator Author

Why not use the existing banner tests, and maybe adjust / extend them a bit? The code there seems very similar to the new helper script. E.g. add extra tests checking the output of is_loaded_directly and maybe just filter these lines for checking the banners later.

I just didn't think about it. But yeah, seems sensible. I'll redo this.

@lgoettgens
Copy link
Collaborator Author

In case of a failure in the banner tests, the CI log will now contain all of the debug output including both backtraces and the line number differences that can be used to extend the @static if-else decision tree.
See e.g. https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/11558837732/job/32172420751?pr=1880#step:6:112 :

Banners: Test Failed at /home/runner/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/utils/Banners/banners.jl:53
  Expression: strip(out) == "Banner of ModB"
   Evaluated: "Banner of ModA\nBanner of ModB" == "Banner of ModB"

Stacktrace:
 [1] top-level scope
   @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/utils/Banners/banners.jl:5
 [2] macro expansion
   @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.12/Test/src/Test.jl:1707 [inlined]
 [3] macro expansion
   @ ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/utils/Banners/banners.jl:53 [inlined]
 [4] macro expansion
   @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.12/Test/src/Test.jl:679 [inlined]
OUT:
Banner of ModA
Banner of ModB

ERR:
┌ Debug: __init__ of ModA
└ @ ModA ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/utils/Banners/ModA/src/ModA.jl:6
┌ Debug: is_loaded_directly: VERSION >= 1.11.0-
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:26
┌ Debug: is_loaded_directly: full backtrace:
│ 22-element Vector{Any}:
│  (is_loaded_directly() at utils.jl:32, 1)
│  (should_show_banner at utils.jl:71 [inlined], 1)
│  (__init__() at ModA.jl:7, 1)
│  (run_module_init(mod::Module, i::Int64) at loading.jl:1419, 1)
│  (register_restored_modules(sv::Core.SimpleVector, pkg::Base.PkgId, path::String) at loading.jl:1407, 1)
│  (_include_from_serialized(pkg::Base.PkgId, path::String, ocachepath::String, depmods::Vector{Any}, ignore_native::Nothing; register::Bool) at loading.jl:1295, 1)
│  (_include_from_serialized at loading.jl:1246 [inlined], 2)
│  (_require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt128, stalecheck::Bool; reasons::Dict{String, Int64}, DEPOT_PATH::Vector{String}) at loading.jl:2101, 1)
│  (_require(pkg::Base.PkgId, env::String) at loading.jl:2603, 1)
│  (__require_prelocked(uuidkey::Base.PkgId, env::String) at loading.jl:2465, 1)
│  (#invoke_in_world#2 at essentials.jl:1083 [inlined], 1)
│  (invoke_in_world at essentials.jl:1080 [inlined], 1)
│  (_require_prelocked(uuidkey::Base.PkgId, env::String) at loading.jl:2452, 1)
│  (macro expansion at loading.jl:2391 [inlined], 1)
│  (macro expansion at lock.jl:294 [inlined], 1)
│  (__require(into::Module, mod::Symbol) at loading.jl:2348, 1)
│  (#invoke_in_world#2 at essentials.jl:1083 [inlined], 1)
│  (invoke_in_world at essentials.jl:1080 [inlined], 1)
│  (require(into::Module, mod::Symbol) at loading.jl:2337, 1)
│  (eval(m::Module, e::Any) at boot.jl:458, 1)
│  (exec_options(opts::Base.JLOptions) at client.jl:295, 1)
│  (_start() at client.jl:558, 1)
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:33
┌ Debug: is_loaded_directly: `_require_search_from_serialized` appears 1 times in backtrace; expected 1
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:36
┌ Debug: is_loaded_directly: `_require_search_from_serialized` called at line 2101, function begins at line 2008, difference 93
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:41
┌ Debug: __init__ of ModB
└ @ ModB ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/test/utils/Banners/ModB/src/ModB.jl:8
┌ Debug: is_loaded_directly: VERSION >= 1.11.0-
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:26
┌ Debug: is_loaded_directly: full backtrace:
│ 22-element Vector{Any}:
│  (is_loaded_directly() at utils.jl:32, 1)
│  (should_show_banner at utils.jl:71 [inlined], 1)
│  (__init__() at ModB.jl:9, 1)
│  (run_module_init(mod::Module, i::Int64) at loading.jl:1419, 1)
│  (register_restored_modules(sv::Core.SimpleVector, pkg::Base.PkgId, path::String) at loading.jl:1407, 1)
│  (_include_from_serialized(pkg::Base.PkgId, path::String, ocachepath::String, depmods::Vector{Any}, ignore_native::Nothing; register::Bool) at loading.jl:1295, 1)
│  (_include_from_serialized at loading.jl:1246 [inlined], 2)
│  (_require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt128, stalecheck::Bool; reasons::Dict{String, Int64}, DEPOT_PATH::Vector{String}) at loading.jl:2[114](https://github.com/Nemocas/AbstractAlgebra.jl/actions/runs/11558837732/job/32172420751?pr=1880#step:6:117), 1)
│  (_require(pkg::Base.PkgId, env::String) at loading.jl:2603, 1)
│  (__require_prelocked(uuidkey::Base.PkgId, env::String) at loading.jl:2465, 1)
│  (#invoke_in_world#2 at essentials.jl:1083 [inlined], 1)
│  (invoke_in_world at essentials.jl:1080 [inlined], 1)
│  (_require_prelocked(uuidkey::Base.PkgId, env::String) at loading.jl:2452, 1)
│  (macro expansion at loading.jl:2391 [inlined], 1)
│  (macro expansion at lock.jl:294 [inlined], 1)
│  (__require(into::Module, mod::Symbol) at loading.jl:2348, 1)
│  (#invoke_in_world#2 at essentials.jl:1083 [inlined], 1)
│  (invoke_in_world at essentials.jl:1080 [inlined], 1)
│  (require(into::Module, mod::Symbol) at loading.jl:2337, 1)
│  (eval(m::Module, e::Any) at boot.jl:458, 1)
│  (exec_options(opts::Base.JLOptions) at client.jl:295, 1)
│  (_start() at client.jl:558, 1)
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:33
┌ Debug: is_loaded_directly: `_require_search_from_serialized` appears 1 times in backtrace; expected 1
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:36
┌ Debug: is_loaded_directly: `_require_search_from_serialized` called at line 2114, function begins at line 2008, difference 106
└ @ AbstractAlgebra ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/utils.jl:41

lgoettgens
lgoettgens commented Oct 28, 2024
View reviewed changes
src/utils.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Collaborator Author

Regarding 1.12: In the current form, the code for is_loaded_directly only works for the current nightly. If it is wished for, I can try to make it work for older nightlies as well, but that seems too much work to do it without reason.
For 1.11 I made sure that it still works with all pre-releases, and extended the list of line number differences to include everything a found so far (with a comment # ignored for older nightlies).

@lgoettgens lgoettgens marked this pull request as ready for review October 28, 2024 17:10
benlorenz
benlorenz approved these changes Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one suggestion which should make it work with some more nightly versions.

src/utils.jl Outdated Show resolved Hide resolved
@lgoettgens @benlorenz
Update src/utils.jl
91af680 
Co-authored-by: Benjamin Lorenz <[email protected]>
@lgoettgens lgoettgens enabled auto-merge (squash) October 29, 2024 09:52
@lgoettgens lgoettgens merged commit 193e1b8 into Nemocas:master Oct 29, 2024
29 checks passed
@lgoettgens lgoettgens deleted the lg/banner branch October 29, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benlorenz benlorenz benlorenz approved these changes

@fingolfin fingolfin Awaiting requested review from fingolfin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lgoettgens @benlorenz